-
Notifications
You must be signed in to change notification settings - Fork 951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test enabling rewards after shielding #3959
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a review of just 1f05f77 . I think this approach is the way to go, thanks! Ah, I see the new MASP integration test is still failing. Let me see if I can spot the error...
My guess is that this issue is similar to having a shielded balance with the single note 1 [2^64 Wei] and trying to spend/send 1 [2^0 Wei] to another payment address. Though these two asset types concern the same underlying token, they are not convertible, and the MASP cannot use/break down the former amount (as far as I remember) to send the much smaller output. You would first need to unshield the 1 [2^64 Wei] note to get a transparent balance of 2^64 Wei, and then shield (2^64 - 1) [2^0 Wei] back to yourself and the other 1 [2^0 Wei] to your desired destination. So essentially the Insufficient funds
error would be correct in a sense. This is my guess based on how this test also broke #3954 ...
If the above guess is correct, then the resolution to this error is to adjust the amounts worked with in the new test to stop straddling the MASP digit boundaries. Or more adventurously, do the unshield-then-shield trick above. But I would lean towards the former solution and make a separate test to demonstrate the above inconvertibility behaviour.
async fn is_amount_required( | ||
&mut self, | ||
client: &(impl Client + Sync), | ||
src: ValueSum<(MaspDigitPos, Address), Change>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ValueSum<(MaspDigitPos, Address), Change>
possibly be changed to ValueSum<(MaspDigitPos, Address), i128>
here? It seems to me that we need to use a type ValueSum<(MaspDigitPos, Address), X>
where X
can hold sums and differences of u64
s. And while X = Change
works, it might be overkill and it might be concerning if X
is using that much more than 64 bits.
&mut self, | ||
client: &(impl Client + Sync), | ||
src: ValueSum<(MaspDigitPos, Address), Change>, | ||
dest: ValueSum<(MaspDigitPos, Address), Change>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ValueSum<(MaspDigitPos, Address), Change>
possibly be changed to ValueSum<(MaspDigitPos, Address), i128>
here?
client: &(impl Client + Sync), | ||
src: ValueSum<(MaspDigitPos, Address), Change>, | ||
dest: ValueSum<(MaspDigitPos, Address), Change>, | ||
delta: I128Sum, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ValueSum<(MaspDigitPos, Address), Change>
possibly be changed to ValueSum<(MaspDigitPos, Address), i128>
here?
src: ValueSum<(MaspDigitPos, Address), Change>, | ||
dest: ValueSum<(MaspDigitPos, Address), Change>, | ||
delta: I128Sum, | ||
) -> Option<ValueSum<(MaspDigitPos, Address), Change>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ValueSum<(MaspDigitPos, Address), Change>
possibly be changed to ValueSum<(MaspDigitPos, Address), i128>
here?
for ((_, asset_data), value) in decoded_delta.components() { | ||
converted_delta += ValueSum::from_pair( | ||
(asset_data.position, asset_data.token.clone()), | ||
Change::from(*value), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would then be 128::from
.
&mut self, | ||
client: &(impl Client + Sync), | ||
added_amt: I128Sum, | ||
mut required_amt: ValueSum<(MaspDigitPos, Address), Change>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ValueSum<(MaspDigitPos, Address), Change>
possibly be changed to ValueSum<(MaspDigitPos, Address), i128>
here?
if value != 0 { | ||
Some(( | ||
MaspDigitPos::from(masp_digit_pos), | ||
Change::from(value), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would then be i128::from
.
Describe your changes
Based on #3954 (DIFF)
This PR accomplishes the following:
Checklist before merging
breaking::
labelsnamada-docs
reponamada-indexer
ornamada-masp-indexer
, a corresponding PR is opened in that repo